Skip to content

fix: #2463 update event handling for field-bitmap to support touch #2492

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
RoboErikG merged 4 commits into from
Feb 24, 2025
Merged

fix: #2463 update event handling for field-bitmap to support touch #2492

RoboErikG merged 4 commits into from
Feb 24, 2025

Conversation

@RoboErikG
Copy link
Contributor

@RoboErikG RoboErikG commented Feb 21, 2025

The basics

The details

Resolves

Fixes #2463 by changing how the editor handles mouse and touch events.

Proposed Changes

Updates the Bitmap field to properly support touch events. This change is based on code-dot-org/code-dot-org#61709 .

  • Switches from handling mouse events to pointer events and moves the event handling up to the editor. This prevents the touch events from getting trapped in an individual pixel.
  • Uses bind instead of conditionalBind. conditionalBind does extra work to account for multi-touch gestures, which causes issues for the editor and isn't needed.
  • Prevents default handling of touchmove (instead of drag) within the editor. The default handling cancels the original gesture when it converts it to a drag, which prevents future move events.
  • Adds a cancel listener to also cleanup the edit state when canceled.
Screen.recording.2025-02-21.9.59.31.AM.webm

Reason for Changes

Touch events in the bitmap editor would get canceled by the browser, causing the gesture to end early and the state to not be cleaned up properly. This would cause Blockly to become partially unresponsive.

Test Coverage

Tested manually on a Chromebook with a touch screen.

Documentation

Additional Information

@RoboErikG RoboErikG requested a review from a team as a code owner February 21, 2025 18:18
@RoboErikG RoboErikG requested review from rachel-fenichel and removed request for a team February 21, 2025 18:18
@rachel-fenichel
Copy link
Collaborator

rachel-fenichel commented Feb 21, 2025

Use npm run format to format and then push again.

@RoboErikG
Copy link
Contributor Author

RoboErikG commented Feb 21, 2025

Hi @mikeharv I made a fix to the bitmap editor based on your change. If you have some time would you mind taking a look to make sure I'm not missing anything?

@rachel-fenichel rachel-fenichel requested review from gonfunko and removed request for rachel-fenichel February 22, 2025 03:13
gonfunko
gonfunko approved these changes Feb 24, 2025
View reviewed changes
plugins/field-bitmap/src/field-bitmap.ts
* @param e The down event.
*/
private onPointerStart(e: PointerEvent) {
const currentElement = document.elementFromPoint(e.clientX, e.clientY);
Copy link
Contributor

@gonfunko gonfunko Feb 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does e.target not work here?

Copy link
Contributor

@gonfunko gonfunko Feb 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And in onPointerMove

Copy link
Contributor Author

@RoboErikG RoboErikG Feb 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Touch events get captured by the element they start on. This means the target is always the element the event started on, so it won't change when you drag across another pixel cell.

@RoboErikG RoboErikG merged commit 73e8819 into RaspberryPiFoundation:master Feb 24, 2025
9 checks passed
@RoboErikG
Copy link
Contributor Author

RoboErikG commented Feb 24, 2025

For completeness, I did find an alternate to using elementFromPoint() would be to cast the target in the pointerdown event to an Element and call releasePointerCapture(). This might have a slight performance advantage, but both options work well so it's not worth revisiting unless we find an issue with the current approach.

      const elem = e.target as Element;
      if (elem && elem.hasPointerCapture(e.pointerId)) {
        elem.releasePointerCapture(e.pointerId);
      }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gonfunko gonfunko gonfunko approved these changes

Assignees

@gonfunko gonfunko

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bitmap field editor freezes after one click on mobile devices

3 participants

@RoboErikG @rachel-fenichel @gonfunko